Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove indirection for builtin operator tests #2109

Merged
merged 13 commits into from
Sep 15, 2022

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Aug 25, 2022

Not sure if there was something behind the indirection...?

@jonmeow jonmeow requested a review from zygoloid August 25, 2022 16:35
@github-actions github-actions bot requested a review from josh11b August 25, 2022 16:35
@jonmeow jonmeow removed the request for review from josh11b August 25, 2022 16:49
junheecho added a commit to junheecho/carbon-lang that referenced this pull request Aug 29, 2022
@zygoloid
Copy link
Contributor

zygoloid commented Sep 2, 2022

The idea of these tests was to make sure these interfaces are properly implemented for builtin types. The revised tests would pass if the interfaces don't work for builtin types but the operators are instead special-cased for those types.

@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 2, 2022

The idea of these tests was to make sure these interfaces are properly implemented for builtin types. The revised tests would pass if the interfaces don't work for builtin types but the operators are instead special-cased for those types.

Can you explain to me how that differs from the existing template code? How does the existing template code force the i32 builtin to be used? (that is, how does it prevent writing an interface for the type if the alternative doesn't?)

@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 6, 2022

// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/operators/mul_builtin.carbon:[[@LINE+2]]: type error in `*`:
// CHECK: could not find implementation of interface MulWith(U = T:! MulWith(i32) where .Self.Result == i32) for T:! MulWith(i32) where .Self.Result == i32
fn DoMul[T:! MulWith(i32) where .Result == i32](x: T, y: T) -> T { return x * y; }

I think I'm starting to understand what you mean, but this makes me wonder if this is behaving as intended...

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks like a simpler and clearer way of testing the same thing.

explorer/testdata/operators/add_builtin.carbon Outdated Show resolved Hide resolved
explorer/testdata/operators/mod_builtin.carbon Outdated Show resolved Hide resolved
@zygoloid
Copy link
Contributor

zygoloid commented Sep 8, 2022

// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/operators/mul_builtin.carbon:[[@LINE+2]]: type error in `*`:
// CHECK: could not find implementation of interface MulWith(U = T:! MulWith(i32) where .Self.Result == i32) for T:! MulWith(i32) where .Self.Result == i32
fn DoMul[T:! MulWith(i32) where .Result == i32](x: T, y: T) -> T { return x * y; }

I think I'm starting to understand what you mean, but this makes me wonder if this is behaving as intended...

I think the problem here was that you're asking for a T that can be multiplied with an i32, but you're multiplying it with a T instead. The error message isn't great but I think explorer is correct to reject this.

jonmeow and others added 3 commits September 8, 2022 16:06
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
@jonmeow
Copy link
Contributor Author

jonmeow commented Sep 14, 2022

@zygoloid Pinging here, did you have any remaining concerns?

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional suggestions to make tests less likely to pass by accident, but LG either way.

explorer/testdata/operators/mod_builtin.carbon Outdated Show resolved Hide resolved
explorer/testdata/operators/mod_builtin.carbon Outdated Show resolved Hide resolved
explorer/testdata/operators/sub_builtin.carbon Outdated Show resolved Hide resolved
explorer/testdata/operators/sub_builtin.carbon Outdated Show resolved Hide resolved
jonmeow and others added 4 commits September 15, 2022 13:51
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
@jonmeow jonmeow merged commit 005ab78 into carbon-language:trunk Sep 15, 2022
@jonmeow jonmeow deleted the builtin-tests branch September 15, 2022 21:36
@chandlerc chandlerc added the explorer Action items related to Carbon explorer code label Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants